Skip to content

Comments

fix : handled checkbox and radio button check on press enter#957

Closed
speshkar-c-eightfold wants to merge 13 commits intoEightfoldAI:mainfrom
speshkar-c-eightfold:speshkar/handleonenter-checkbox-radio
Closed

fix : handled checkbox and radio button check on press enter#957
speshkar-c-eightfold wants to merge 13 commits intoEightfoldAI:mainfrom
speshkar-c-eightfold:speshkar/handleonenter-checkbox-radio

Conversation

@speshkar-c-eightfold
Copy link
Contributor

SUMMARY:

checkboxes and radio buttons were not accessible on enter button

GITHUB ISSUE (Open Source Contributors)

JIRA TASK (Eightfold Employees Only):

https://eightfoldai.atlassian.net/browse/ENG-116412
https://eightfoldai.atlassian.net/browse/ENG-116416

CHANGE TYPE:

  • Bugfix Pull Request
  • Feature Pull Request

TEST COVERAGE:

  • Tests for this change already exist
  • I have added unittests for this change

TEST PLAN:

Test Case 1 :
Go to storybook check if checkboxes are getting checked and unchecked on press Enter key

Screen.Recording.2025-03-06.at.3.54.55.PM.mov

Test Case 2 :
Chcek if radio Buttons are getting checked on enter key press

Screen.Recording.2025-03-06.at.3.56.09.PM.mov

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 6, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@ypatadia-eightfold ypatadia-eightfold self-requested a review March 13, 2025 07:47
Comment on lines 185 to 189
setIsChecked((prev) => !prev);
onChange?.({
...(e as any),
currentTarget: { checked: !target.checked },
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we resuse toggleChecked()?

Comment on lines 175 to 180
const target = e.target as HTMLInputElement;
setIsActive((prev) => !prev);
onChange?.({
...(e as any),
currentTarget: { checked: !target.checked },
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we resuse toggleChecked()?

const target = e.target as HTMLInputElement;
setIsActive((prev) => !prev);
onChange?.({
...(e as any),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't use any

Comment on lines +183 to 201
const applyCheckedState = (checked: boolean) => {
setIsChecked(checked);
onChange?.({
target: { checked, type: 'checkbox' } as HTMLInputElement,
currentTarget: { checked, type: 'checkbox' } as HTMLInputElement,
type: 'change',
} as React.ChangeEvent<HTMLInputElement>);
};

const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>): void => {
if (e.key === 'Enter' || e.key === ' ') {
e.preventDefault();
applyCheckedState(!isChecked);
}
};

const toggleChecked = (e: React.ChangeEvent<HTMLInputElement>): void => {
setIsChecked(e.target.checked);
onChange?.(e);
applyCheckedState(e.target.checked);
};
Copy link
Collaborator

@ypatadia-eightfold ypatadia-eightfold Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>): void => {
      if (e.key === 'Enter' || e.key === ' ') {
        e.preventDefault();
        const syntheticEvent = {
          ...e,
          currentTarget: {
            ...e.currentTarget,
            checked: !isChecked,
          } as HTMLInputElement,
          target: {
            ...e.target,
            checked: !isChecked,
          } as HTMLInputElement,
        } as React.ChangeEvent<HTMLInputElement>;
        setIsChecked(!isChecked);
        onChange?.(syntheticEvent);
      }
    };
    const toggleChecked = (e: React.ChangeEvent<HTMLInputElement>): void => {
      setIsChecked(e.target.checked);
      onChange?.(e);
    };

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please test these out. This implmentation for checkbox and radio ensures consistent implementation across both components.

Comment on lines +163 to 194
const applyRadioChange = (
value: string,
e: React.SyntheticEvent<HTMLInputElement>
) => {
if (!radioGroupContext) {
setSelectedValue(e.currentTarget?.value as RadioButtonValue);
setSelectedValue(value as RadioButtonValue);
} else {
radioGroupContext?.onChange?.(e);
radioGroupContext.onChange?.(e as React.ChangeEvent<HTMLInputElement>);
}

onChange?.(e);
onChange?.(e as React.ChangeEvent<HTMLInputElement>);
};

const toggleChecked = (e: React.ChangeEvent<HTMLInputElement>): void => {
applyRadioChange(e.currentTarget.value, e);
};

const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>): void => {
if (e.key === 'Enter' || e.key === ' ') {
const target = e.target as HTMLInputElement;

setIsActive((prev) => !prev);

const syntheticEvent = {
...e,
currentTarget: target,
target: target,
} as unknown as React.ChangeEvent<HTMLInputElement>;

applyRadioChange(target.value, syntheticEvent);
}
};
Copy link
Collaborator

@ypatadia-eightfold ypatadia-eightfold Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const toggleChecked = (e: React.ChangeEvent<HTMLInputElement>): void => {
      if (!radioGroupContext) {
        setSelectedValue(e.currentTarget?.value as RadioButtonValue);
      } else {
        radioGroupContext?.onChange?.(e);
      }

      onChange?.(e);
    };
    const handleKeyDown = (e: React.KeyboardEvent<HTMLInputElement>): void => {
      if (e.key === 'Enter' || e.key === ' ') {
        e.preventDefault();
        const syntheticEvent = {
          ...e,
          currentTarget: {
            ...e.currentTarget,
            checked: true,
            value: value,
          } as HTMLInputElement,
          target: {
            ...e.target,
            checked: true,
            value: value,
          } as HTMLInputElement,
        } as React.ChangeEvent<HTMLInputElement>;
        
        toggleChecked(syntheticEvent);
      }
    };

@speshkar-c-eightfold
Copy link
Contributor Author

Closing this PR as checkboxes should not be handled on Enter key press whereas radio buttons work as expected . Moving these tickets to QA testing .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants